o/c/c/ntp: integrate NTP configuration options into snapd by lorenzo-medici · Pull Request #17022 · canonical/snapd

lorenzo-medici · 2026-05-05T13:14:56Z

This is still a draft. For example, tests are missing.

This PR adds a set of configuration options to the core snap to allow control over the NTP utilities:

core.system.ntp.servers
core.system.ntp.fallback-servers
core.system.ntp.root-distance-max-sec
core.system.ntp.poll-interval-min-sec
core.system.ntp.poll-interval-max-sec
core.system.ntp.connection-retry-sec
core.system.ntp.save-interval-sec

These map to the systemd-timesyncd.service configuration keys in /etc/systemd/timesyncd.conf keys like so:

var snapToTimesyncdKeyMapping = map[string]string{
	"servers":               "NTP",
	"fallback-servers":      "FallbackNTP",
	"root-distance-max-sec": "RootDistanceMaxSec",
	"poll-interval-min-sec": "PollIntervalMinSec",
	"poll-interval-max-sec": "PollIntervalMaxSec",
	"connection-retry-sec":  "ConnectionRetrySec",
	"save-interval-sec":     "SaveIntervalSec",
}

The configuration is stored on disk. Whenever it is retrieved, it is read from the file, parsed and transformed into a JSON-like object.

When it is set, the entries are validated and the approved configuration is stored on disk. The service is then reloaded.

The keys are validated for format and values. The two servers keys must be a list of strings, whose items must be valid IP addresses or network names.
The other keys must be valid systemd timespans or positive integers (defaulting to seconds). Additionally, poll-interval-min-sec must be greater than 16s and lower than poll-interval-max-sec, and connection-retry-sec must be greater than 1s.

This has been tested manually in a VM and the operations validated by checking the contents of /etc/systemd/timesyncd.conf and the output of timedatectl show-timesync. Unit tests are still pending.

References:

github-actions · 2026-05-05T13:25:06Z

Thu Jul 2 14:32:12 UTC 2026
The following results are from: https://github.com/canonical/snapd/actions/runs/28596102772

No spread failures reported

lorenzo-medici · 2026-05-08T14:58:53Z

I changed the PR name after pushing so the static checks failed because of it and nothing else was run after that.
Could someone re-trigger the tests?

Edit: the new test run didn't pick up the updated PR name. I force-pushed with an identical commit with different date to trigger the complete workflow.

codecov · 2026-05-12T08:28:11Z

Codecov Report

❌ Patch coverage is 94.77352% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.15%. Comparing base (57190d4) to head (49db17e).
⚠️ Report is 81 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/configstate/configcore/ntp.go	94.71%	11 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17022      +/-   ##
==========================================
- Coverage   79.17%   79.15%   -0.03%     
==========================================
  Files        1377     1384       +7     
  Lines      193225   194801    +1576     
  Branches     2466     2466              
==========================================
+ Hits       152989   154186    +1197     
- Misses      31056    31364     +308     
- Partials     9180     9251      +71

Flag	Coverage Δ
unittests	`79.15% <94.77%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR introduces core.system.ntp.* configuration options for Ubuntu Core systems and wires them into configcore so snapd can read/write systemd-timesyncd settings (via /etc/systemd/timesyncd.conf) and apply changes.

Changes:

Add a new configcore NTP module that validates NTP options, serializes them to a systemd-style config, and exposes the current system configuration via an external config getter.
Add unit tests covering valid/invalid configurations and error paths.
Register the new NTP handler in configcore filesystem-only handlers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.

File	Description
overlord/configstate/configcore/ntp.go	Implements NTP option validation, (de)serialization, external config getter, and application logic (write config + reload/restart timesyncd).
overlord/configstate/configcore/ntp_test.go	Adds gocheck tests for validation, file content updates, and error handling.
overlord/configstate/configcore/handlers.go	Registers the new `system.ntp` filesystem-only handler.

pedronis

we need to be consistent with other duration options

pedronis · 2026-06-04T12:46:08Z

+			return fmt.Errorf("%v: %v", key, err)
+		}
+
+		timespanUs, err := validateSystemdTimeSpanFormat(span)


we use golang durations in other system options, we need to be consistent with that here, and also find namings that don't have "-sec" in the option names

These values end up being written to /etc/systemd/timesyncd.conf, and should be in systemd.time format.

Regarding the option names, they are a snap-friendly formatting of the timesyncd config names. I have renamed the options to slightly clearer values:

var timesyncdToSnapKeyMapping = map[string]string{ "NTP": "servers", "FallbackNTP": "fallback-servers", "RootDistanceMaxSec": "max-root-time-distance", "PollIntervalMinSec": "min-poll-interval", "PollIntervalMaxSec": "max-poll-interval", "ConnectionRetrySec": "connection-retry-interval", "SaveIntervalSec": "save-interval", }

Let me know if some need to be changed. I will push with the next set of commits.

[1] man timesyncd.conf
[2] man systemd.time

they can't be because we don't use that format anywhere else, you need convert from go duration to what you need there

Oh I understand now, you mean for the input values that the user set's and get's.
I will add the translation steps.

lorenzo-medici · 2026-06-09T07:56:48Z

A dependency I needed was removed from go.mod in master, making the CI fail to build. I rebased the branch and added the module back, so I needed to force-push.

Edit: and again

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…l, etc. Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…ng improvements, bugfixes Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…onfiguration Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…keys in config file, expand test coverage Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…r list parsing Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…e operations Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…clude all of them Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…gs to go time.Duration strings Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

valentindavid · 2026-06-18T14:50:33Z

+}
+
+// Too simplistic?
+var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`).MatchString


Is that something that will delay start of snapd? I think @bboozzoo was looking into improving start time, but not sure.

[a-zA-Z0-9\-] -> [a-zA-Z0-9-] should work.

Does it really have to be a host name? ipv6 address are not matched by it.

valentindavid · 2026-06-18T14:59:21Z

+var validIPv4 = net.ParseIP
+
+// Match the line containing "μs:" or "us:" and capture the following digits
+var timespanUsRegexp = regexp.MustCompile(`(?:μs|us):\s*(\d+)`)


Non-ascii? Could we use "us" instead?

Verified on UC24:

# systemd-analyze timespan 1d Original: 1d μs: 86400000000 Human: 1d

bboozzoo · 2026-06-19T05:51:35Z

+}
+
+func validateServerName(serverAddress string) error {
+	if validIPv4(serverAddress) == nil && !validHostname(serverAddress) {


what if my NTP server is accessible over IPv6?

bboozzoo · 2026-06-19T05:52:14Z

+
+// Too simplistic?
+var validHostname = regexp.MustCompile(`^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])(\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]{0,61}[a-zA-Z0-9]))*$`).MatchString
+var validIPv4 = net.ParseIP


this actually parses ipv6 as well and there's no need to use a variable

bboozzoo · 2026-06-19T06:03:31Z

+	// Validation for user submitted values has already been done, so ParseDuration cannot fail here.
+	if minVal, exists := ntpCfg["min-poll-interval"]; exists {
+		pollIntervalMinString = minVal.(string)
+		pollIntervalMin, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMinString))


Suggested change

pollIntervalMin, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMinString))

pollIntervalMin, _ = time.ParseDuration(pollIntervalMinString)

bboozzoo · 2026-06-19T06:03:43Z

+	}
+	if maxVal, exists := ntpCfg["max-poll-interval"]; exists {
+		pollIntervalMaxString = maxVal.(string)
+		pollIntervalMax, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMaxString))


Suggested change

pollIntervalMax, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMaxString))

pollIntervalMax, _ = time.ParseDuration(pollIntervalMaxString)

bboozzoo · 2026-06-19T06:04:02Z

+
+		// The value that the user inputs should be parsed as a Go duration string for consistency
+		// with other configuration options
+		duration, err := time.ParseDuration(strings.TrimSpace(valueStr))


Suggested change

duration, err := time.ParseDuration(strings.TrimSpace(valueStr))

duration, err := time.ParseDuration(valueStr)

side note, it's probably worth checking that the input does not contain extra whitespace

bboozzoo · 2026-06-19T06:34:37Z

+	return byteStream
+}
+
+func getNTPFromSystemHelper(key string) (result any, err error) {


should key be ignored?

I followed the flow from the netplan config, which also ignores the key.

bboozzoo · 2026-06-19T06:37:09Z

+
+func (s *ntpSuite) TestNTPSetRestartDaemonError(c *C) {
+	r := systemd.MockSystemctl(func(cmd ...string) ([]byte, error) {
+		s.sysdLog = append(s.sysdLog, cmd)


not checked in the test?

bboozzoo · 2026-06-19T06:37:54Z

+func (s *ntpSuite) SetUpTest(c *C) {
+	s.configcoreSuite.SetUpTest(c)
+
+	c.Assert(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "etc/systemd"), 0755), IsNil)


this is done again just below

Suggested change

c.Assert(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, "etc/systemd"), 0755), IsNil)

bboozzoo · 2026-06-19T06:38:55Z

+	for i, test := range getConfigurationTests {
+		// Apply the config
+		conf := configcore.PlainCoreConfig(test.newConfig)
+		err := configcore.FilesystemOnlyRun(core24Dev, conf)


won't this call systemd-analyze?

It will. The existing configuration is read to identify changes and the need to restart the timesyncd daemon. In the reading logic in getNTPFromSystem, systemd-analyze is used to convert systemd timespans into go Durations.

For this reason, I also cannot add the "exit 1" snippet mocking systemd-analyze.
The error paths are already covered, but I can mock it if you think it best.

bboozzoo · 2026-06-19T06:40:20Z

+	// We are on Core
+	restore := release.MockOnClassic(false)
+	s.AddCleanup(restore)
+}


can you add a snippet like:

cmd = testutil.MockCommand(c, "systemd-analyze", "echo 'systemd-analyze not mocked'; exit 1") s.AddCleanup(cmd.Restore()

* replaced `\-` with `-` in regex for server address validation * removed `validIPv4` variable and directly used `net.ParseIP` as it supports IPv6 * added test with IPv6 server being validated. Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…r trailing whitespace Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Meulengracht

First pass, some comments also this should have a spread test.

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…Helper Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…nap configs Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Meulengracht · 2026-07-02T09:32:45Z

+    [ "$ts_before" != "$(get_restart_ts)" ]
+
+    echo "snap get reports no configuration option again after the reset"
+    not snap get system system.ntp


missing newline

Meulengracht · 2026-07-02T09:35:34Z

+    echo "Updating a single option preserves the other configured options"
+    ts_before="$(get_restart_ts)"
+    snap set system system.ntp.min-poll-interval=1m
+    MATCH '^PollIntervalMinSec=1m$' < "$NTP_CONF"
+    MATCH '^NTP=192.168.1.1 ntp.ubuntu.com$' < "$NTP_CONF"
+    MATCH '^SaveIntervalSec=20s$' < "$NTP_CONF"
+    [ "$ts_before" != "$(get_restart_ts)" ]


I think this should atleast assert one additional non updated key, maybe FallbackNTP to ensure that we actually preserve the field correctly.

Meulengracht · 2026-07-02T09:36:36Z

+		unitOptions = append(unitOptions, &unitOption)
+	}
+
+	byteStream, _ := io.ReadAll(unit.Serialize(unitOptions))


Needs a comment on why we are ignoring the error here

Meulengracht · 2026-07-02T09:43:36Z

+		return nil, nil
+	}
+
+	file, err := os.Open(filepath.Join(dirs.GlobalRootDir, "etc", "systemd", "timesyncd.conf"))


If we decide to ignore drop-ins, but we need to document this behaviour, both here in the code but also as a part of the forward-facing docs - but since this is core the presence of drop ins can be relatively limited, is there a requirement on this behaviour from what prompted this feature? @lorenzo-medici

…sful set operation Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

…d not using drop-ins for timesyncd configuration Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

github-actions Bot added Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run only one system Only runs spread tests on one system labels May 5, 2026

alfonsosanchezbeato requested review from Meulengracht and Mohit-Chachada May 5, 2026 13:29

lorenzo-medici changed the title ~~[DRAFT] Integrate NTP configuration options into snapd~~ o/c/c/ntp: integrate NTP configuration options into snapd May 8, 2026

lorenzo-medici marked this pull request as ready for review May 8, 2026 14:45

github-actions Bot removed the Run only one system Only runs spread tests on one system label May 8, 2026

lorenzo-medici force-pushed the integrate-ntp-configuration branch from 3f858d7 to ef6b868 Compare May 12, 2026 08:01

ndyer requested review from valentindavid and removed request for Mohit-Chachada May 19, 2026 12:42

Meulengracht requested a review from Copilot June 1, 2026 12:45

Copilot started reviewing on behalf of Meulengracht June 1, 2026 12:45 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Meulengracht requested a review from pedronis June 3, 2026 08:54

Meulengracht added the Needs Samuele review Needs a review from Samuele before it can land label Jun 3, 2026

pedronis requested changes Jun 4, 2026

View reviewed changes

lorenzo-medici force-pushed the integrate-ntp-configuration branch 2 times, most recently from ba3b38a to c01a6af Compare June 9, 2026 07:54

lorenzo-medici added 8 commits June 12, 2026 17:27

o/c/c/ntp: configure NTP servers from snapd options

6f00cc7

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: configure additional NTP options RootDistance, PollInterva…

be4e535

…l, etc. Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: skip writing the configuration if it has not changed, nami…

d4bc54c

…ng improvements, bugfixes Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp_test: added tests for getting and setting core.system.ntp c…

067e83c

…onfiguration Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: support unsetting system.ntp key by deleting the config file

ad1be51

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: removed validation in handle function, ignore unsupported …

86af750

…keys in config file, expand test coverage Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp_test: fixed import errors in unit tests

6a7840e

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: improved handling of empty options in configuration file

0f4b81f

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

lorenzo-medici added 10 commits June 12, 2026 17:27

o/c/c/ntp_test: improve unit test coverage

f631487

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: do not restart systemd-timesyncd during image build

c48427f

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: make NTPConfigurationDeepEqual unexported

e602fbb

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: replace strings.Split with strings.Fields to improve serve…

86adba6

…r list parsing Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp_test: make tests more robust by checking more errors on fil…

542da8d

…e operations Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: improve string formatting

b3d660e

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/handlers: comment for NTP options changed to system.ntp.* to in…

4941937

…clude all of them Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: rename snap config options to improve clarity

6e6010e

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: input format for intervals changed from systemd.time strin…

fdf9fcc

…gs to go time.Duration strings Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

go.mod: added github.com/coreos/go-systemd module

8d229f4

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

lorenzo-medici force-pushed the integrate-ntp-configuration branch from 72fad8b to 8d229f4 Compare June 12, 2026 14:33

lorenzo-medici requested a review from pedronis June 12, 2026 15:12

valentindavid reviewed Jun 18, 2026

View reviewed changes

bboozzoo reviewed Jun 19, 2026

View reviewed changes

lorenzo-medici added 2 commits June 19, 2026 15:40

o/c/c/ntp: user-provided strings are rejected if containing leading o…

a79fb61

…r trailing whitespace Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

ndyer assigned Meulengracht Jun 25, 2026

o/c/c/ntp: test cleanup

16e4ecb

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Meulengracht reviewed Jun 30, 2026

View reviewed changes

lorenzo-medici added 4 commits June 30, 2026 16:14

o/c/c/ntp: reject negative duration values explicitly with clear message

1bc2c4f

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: simplify function comparing {min,max}-poll-interval values

552aed5

Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: added docstrings for getNTPFromSystem and getNTPFromSystem…

30b0226

…Helper Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

tests/core: added spread test for setting timesyncd options through s…

49db17e

…nap configs Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

Meulengracht approved these changes Jul 2, 2026

View reviewed changes

lorenzo-medici added 2 commits July 2, 2026 16:35

tests/core/ntp-config: check additional unmodified field after succes…

01ed5e5

…sful set operation Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

o/c/c/ntp: added comments about writing to main configuration file an…

9429cf1

…d not using drop-ins for timesyncd configuration Signed-off-by: Lorenzo Medici <lorenzo.medici@canonical.com>

	pollIntervalMin, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMinString))
	pollIntervalMin, _ = time.ParseDuration(pollIntervalMinString)

	pollIntervalMax, _ = time.ParseDuration(strings.TrimSpace(pollIntervalMaxString))
	pollIntervalMax, _ = time.ParseDuration(pollIntervalMaxString)

	duration, err := time.ParseDuration(strings.TrimSpace(valueStr))
	duration, err := time.ParseDuration(valueStr)

Uh oh!

Conversation

lorenzo-medici commented May 5, 2026

References:

Uh oh!

github-actions Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No spread failures reported

Uh oh!

lorenzo-medici commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzo-medici commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions Bot commented May 5, 2026 •

edited

Loading

lorenzo-medici commented May 8, 2026 •

edited

Loading

codecov Bot commented May 12, 2026 •

edited

Loading

lorenzo-medici commented Jun 9, 2026 •

edited

Loading